Decouple policy logic from resource url for getSignedUrlWithCustomPolicy #5862

RanVaknin · 2025-02-04T21:52:33Z

Motivation and Context

Fixes #5577 where users cannot specify a wildcard (*) resource URL policy separate from the actual URL to be signed
Current implementation couples the resource URL for both signing and policy specification
This change allows for more flexible URL signing scenarios.

Modifications

Added new resourceUrlPattern field to CustomSignerRequest
Updated CloudFrontUtilities.getSignedUrlWithCustomPolicy to use resourceUrlPattern when specified

Testing

Added a unit test to verify resourceUrlPattern applies the correct logic to the policy.
Added integration tests to verify real-world scenarios: (Passing on intg test acct ✅ )

Testing with query parameters
Testing with wildcard paths
Testing with full wildcard policy

...ront/src/main/java/software/amazon/awssdk/services/cloudfront/model/CustomSignerRequest.java

...test/java/software/amazon/awssdk/services/cloudfront/CloudFrontUtilitiesIntegrationTest.java

...cloudfront/src/main/java/software/amazon/awssdk/services/cloudfront/CloudFrontUtilities.java

...ront/src/main/java/software/amazon/awssdk/services/cloudfront/model/CustomSignerRequest.java

...cloudfront/src/main/java/software/amazon/awssdk/services/cloudfront/CloudFrontUtilities.java

...ront/src/main/java/software/amazon/awssdk/services/cloudfront/model/CustomSignerRequest.java

joviegas · 2025-02-12T18:16:33Z

...dfront/src/test/java/software/amazon/awssdk/services/cloudfront/CloudFrontUtilitiesTest.java

@@ -331,4 +334,44 @@ void getCookiesForCustomPolicy_withActiveDateAndIpRangeOmitted_producesValidCook
        assertThat(cookiesForCustomPolicy.keyPairIdHeaderValue()).isEqualTo("CloudFront-Key-Pair-Id=keyPairId");
    }

+    @Test


Can we add a parameterized test to test different possibilities of resourceUrlPattern ?
Something like

@ParameterizedTest @MethodSource("provideUrlPatternsAndExpectedResources") void getSignedURLWithCustomPolicy_policyResourceUrlShouldHandleVariousPatterns( String resourceUrlPattern, String expectedResource) { String baseUrl = "https://d1234.cloudfront.net/images/photo.jpg"; Instant expiration = Instant.now().plusSeconds(3600); CustomSignerRequest request = CustomSignerRequest.builder() .resourceUrl(baseUrl) .privateKey(keyPair.getPrivate()) .keyPairId("keyPairId") .resourceUrlPattern(resourceUrlPattern) .expirationDate(expiration) .build(); SignedUrl signedUrl = cloudFrontUtilities.getSignedUrlWithCustomPolicy(request); // Extract and decode the policy String encodedPolicy = signedUrl.url().split("Policy=")[1].split("&")[0]; String standardBase64 = encodedPolicy .replace('-', '+') .replace('_', '=') .replace('~', '/'); String decodedPolicy = new String(Base64.getDecoder().decode(standardBase64), UTF_8); // Build expected policy StringBuilder expectedPolicy = new StringBuilder(); StringJoiner conditions = new StringJoiner(",", "{", "}"); conditions.add("\"DateLessThan\":{\"AWS:EpochTime\":" + expiration.getEpochSecond() + "}"); expectedPolicy.append("{\"Statement\": [{") .append("\"Resource\":\"").append(expectedResource).append("\",") .append("\"Condition\":").append(conditions) .append("}]}"); assertThat(decodedPolicy.trim()).isEqualTo(expectedPolicy.toString().trim()); // Verify URL is properly encoded assertThat(signedUrl.url()).doesNotContain(" "); assertThat(signedUrl.url()).matches("^[\\x20-\\x7E]+$"); // Printable ASCII characters only } private static Stream<Arguments> provideUrlPatternsAndExpectedResources() { return Stream.of( // Basic patterns Arguments.of("*", "*"), Arguments.of("https://d1234.cloudfront.net/*", "https://d1234.cloudfront.net/*"), Arguments.of("https://d1234.cloudfront.net/images/*", "https://d1234.cloudfront.net/images/*"), // Specific file patterns Arguments.of("234.cloudfront.net/images/photo.jpg", "https://d1234.cloudfront.net/images/photo.jpg"), Arguments.of("https://d1234.cloudfront.net/*/photo.jpg", "234.cloudfront.net/*/photo.jpg"), // Multiple wildcards Arguments.of("https://*.cloudfront.net/*/photo.jpg", "https://*.cloudfront.net/*/photo.jpg"), // Patterns with special characters that need encoding Arguments.of("https://d1234.cloudfront.net/images/photo with spaces.jpg", "https://d1234.cloudfront.net/images/photo%20with%20spaces.jpg"), Arguments.of("https://d1234.cloudfront.net/images/photo+with+plus.jpg", "234.cloudfront.net/images/photo%2Bwith%2Bplus.jpg"), Arguments.of("https://d1234.cloudfront.net/images/photo?param=value", "234.cloudfront.net/images/photo%3Fparam%3Dvalue"), // Unicode characters Arguments.of("https://d1234.cloudfront.net/images/photo-ümlaut.jpg", "https://d1234.cloudfront.net/images/photo-%C3%BCmlaut.jpg"), // Path traversal patterns Arguments.of("https://d1234.cloudfront.net/images/../photo.jpg", "https://d1234.cloudfront.net/images/../photo.jpg"), // Patterns with query parameters Arguments.of("https://d1234.cloudfront.net/images/photo.jpg?size=large&format=png", "https://d1234.cloudfront.net/images/photo.jpg%3Fsize%3Dlarge%26format%3Dpng")

The parameterized test suite is testing for URL encoding transformations that don't exist in the code. In getSignedUrlWithCustomPolicy, the resource pattern is inserted directly into the policy document as a JSON string without modification, and URL-safe encoding (makeStringUrlSafe) happens later to the entire Base64-encoded policy. The test cases expect the Resource field in the policy to contain URL-encoded values (like %20 for spaces), but this is incorrect since the policy is a JSON document, not a URL, and these transformations never occur in the codepath.

For this reason, in this test suite's case, both the Input and Output arguments will be the same every time.

Thanks Ran
I saw that some how the current implementation on resourceUrl does not handle special cases like below

In CloudFrontUtilitiesIntegrationTest I updated S3_OBJECT_KEY_ON_SUB_PATH to have special charatcters and also updated getSignedUrlWithCustomPolicy_wildCardPath test to cehck for specific url

private static final String S3_OBJECT_KEY_ON_SUB_PATH = "foo/specific space+plus-üspecial"; @Test void getSignedUrlWithCustomPolicy_wildCardPath() throws Exception { System.out.println("bucket® "+bucket); String resourceUri = "https://" + domainName; Instant expirationDate = LocalDate.of(2050, 1, 1) .atStartOfDay() .toInstant(ZoneOffset.of("Z")); Instant activeDate = LocalDate.of(2022, 1, 1) .atStartOfDay() .toInstant(ZoneOffset.of("Z")); CustomSignerRequest request = CustomSignerRequest.builder() .resourceUrl(resourceUri + "/foo/specific space+plus-üspecial") .privateKey(keyFilePath) .keyPairId(keyPairId) .policyResource(resourceUri + "/foo/specific space+plus-üspecial") .activeDate(activeDate) .expirationDate(expirationDate) .build(); SignedUrl signedUrl = cloudFrontUtilities.getSignedUrlWithCustomPolicy(request); URI modifiedUri = URI.create(signedUrl.url().replace("/specific space+plus-üspecial","/other-file")); SdkHttpClient client = ApacheHttpClient.create(); HttpExecuteResponse response = client.prepareRequest(HttpExecuteRequest.builder() .request(SdkHttpRequest.builder() .encodedPath(modifiedUri.getRawPath() + "?" + modifiedUri.getRawQuery()) .host(modifiedUri.getHost()) .method(SdkHttpMethod.GET) .protocol("https") .build()) .build()).call(); assertThat(response.httpResponse().statusCode()).isEqualTo(200); }

But I get

java.lang.IllegalArgumentException: Illegal character in path at index 50: https://d3j5oja9i0ormb.cloudfront.net/foo/specific space+plus-üspecial

We need not fix this in this PR but raise an internal task and discuss this.

Sounds good. I can create a new task on next steps.

For reference, both the JS SDK v3 and Go SDK v2 rely on the standard library's URL parsing, and the provided URLs get properly encoded:
Go SDK impl https://github.com/aws/aws-sdk-go-v2/blob/main/feature/cloudfront/sign/sign_url.go#L194

func main() { rawURL := "https://example.cloudfront.net/foo/specific space+plus-üspecial" uri, err := url.Parse(rawURL) if err != nil { fmt.Println("Error:", err) } else { fmt.Println("Parsed successfully", uri) } } // Parsed successfully https://example.cloudfront.net/foo/specific%20space+plus-%C3%BCspecial

JS SDK impl https://github.com/aws/aws-sdk-js-v3/blob/main/packages/cloudfront-signer/src/sign.ts#L126

try { const uri = new URL("https://d3j5oja9i0ormb.cloudfront.net/foo/specific space+plus-üspecial") console.log("Parsed ", uri.href) } catch (e) { console.log(e); } // "Parsed https://d3j5oja9i0ormb.cloudfront.net/foo/specific%20space+plus-%C3%BCspecial

sonarqubecloud · 2025-02-17T18:44:24Z

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
83.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

zoewangg · 2025-02-18T17:34:44Z

...cloudfront/src/main/java/software/amazon/awssdk/services/cloudfront/CloudFrontUtilities.java

-            String resourceUrl = request.resourceUrl();
-            String policy = SigningUtils.buildCustomPolicyForSignedUrl(request.resourceUrl(), request.activeDate(),
-                                                                       request.expirationDate(), request.ipRange());
+            String resourceUrlPattern = StringUtils.isEmpty(request.resourceUrlPattern())


Is there any reason we are treating empty as null?

Because resourceUrlPattern maps to the JSON Policy document's Resource field, if a user provides null or "" we default to the resourceUrl itself - meaning, there is no pattern, just use the base url as the Resource field in the policy.

The server side implication of send a policy with a Resource field set to "" will result in a 403. I can't foresee a situation in which users are actively prohibiting all access to their distribution just by passing "".

For example:

CustomSignerRequest request = CustomSignerRequest.builder() .resourceUrl(resourceUrl) .privateKey(keyFilePath) .keyPairId(keyPairId) .resourceUrlPattern("") .activeDate(activeDate) .expirationDate(expirationDate) .build();

Will result in a 403.

Right, my question is more around: is this something we want to handle in the SDK? Should we just let it fail on the server side instead?

My concern is that some customers may not realize "" is not supported or have bugs in their code to pass empty string to resourceUrlPattern unintentionally, which will be hidden by this SDK logic.

Ack. I can just use a standard null check.

zoewangg · 2025-02-18T17:48:28Z

.changes/next-release/bugfix-AmazonCloudfront-1fa6f75.json

+    "type": "bugfix",
+    "category": "Amazon Cloudfront",
+    "contributor": "",
+    "description": "Decouple policy logic from resource url for getSignedUrlWithCustomPolicy"


This seems to be implementation detail.

Suggesting:

Allow users to specify resource URL pattern in `CloudFrontUtilities#getSignedUrlWithCustomPolicy`. See [#5577](https://github.com/aws/aws-sdk-java-v2/issues/5577)

Agreed. Do you want me to raise a PR to amend the changelog?

Yeah, that'd be great. It may be easier after release is finished.

RanVaknin requested a review from a team as a code owner February 4, 2025 21:52

davidh44 reviewed Feb 5, 2025

View reviewed changes

...ront/src/main/java/software/amazon/awssdk/services/cloudfront/model/CustomSignerRequest.java Show resolved Hide resolved

...test/java/software/amazon/awssdk/services/cloudfront/CloudFrontUtilitiesIntegrationTest.java Show resolved Hide resolved

joviegas reviewed Feb 8, 2025

View reviewed changes

RanVaknin added 8 commits February 11, 2025 12:41

Decouple policy logic from resource url for getSignedUrlWithCustomPolicy

ba2fe99

Adding validation for resourceUrl to avoid NPE

d6a3f5a

Add changelog

94519ac

Fix checkstyle

c6823a3

Renaming resource policy, adding documentation, refactoring

e67972e

Adding not null validations

0d37a62

Changing resources name for intg test suite recreation

766c3cd

Renaming policyResource, adding a test

95dd934

RanVaknin force-pushed the rvaknin/cloudfront-wildcard-policy branch from 9335163 to 95dd934 Compare February 11, 2025 20:45

joviegas reviewed Feb 12, 2025

View reviewed changes

Adding parameterized test

b10ef40

joviegas approved these changes Feb 16, 2025

View reviewed changes

Fix checkstyle

a9c5755

RanVaknin added this pull request to the merge queue Feb 18, 2025

zoewangg reviewed Feb 18, 2025

View reviewed changes

Merged via the queue into master with commit 5fd2e6e Feb 18, 2025
17 checks passed

RanVaknin mentioned this pull request Feb 19, 2025

Fixing cloudfront signer logic, and changelog #5892

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple policy logic from resource url for getSignedUrlWithCustomPolicy #5862

Decouple policy logic from resource url for getSignedUrlWithCustomPolicy #5862

RanVaknin commented Feb 4, 2025 •

edited

Loading

joviegas Feb 12, 2025

RanVaknin Feb 14, 2025 •

edited

Loading

joviegas Feb 16, 2025

RanVaknin Feb 17, 2025

sonarqubecloud bot commented Feb 17, 2025

zoewangg Feb 18, 2025

RanVaknin Feb 18, 2025

zoewangg Feb 18, 2025

RanVaknin Feb 18, 2025 •

edited

Loading

zoewangg Feb 18, 2025

RanVaknin Feb 18, 2025

zoewangg Feb 18, 2025

Decouple policy logic from resource url for getSignedUrlWithCustomPolicy #5862

Decouple policy logic from resource url for getSignedUrlWithCustomPolicy #5862

Conversation

RanVaknin commented Feb 4, 2025 • edited Loading

Motivation and Context

Modifications

Testing

Choose a reason for hiding this comment

RanVaknin Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 17, 2025

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanVaknin Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanVaknin commented Feb 4, 2025 •

edited

Loading

RanVaknin Feb 14, 2025 •

edited

Loading

RanVaknin Feb 18, 2025 •

edited

Loading